-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve assemblies from specific paths #567
Conversation
This introduces a command-line option, "--ref <path>", which makes it possible to give the linker full assembly paths to resolve. LinkTask now uses "--ref" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves. When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands).
This is necessary for the '--ref' argument to work with the mono build of the linker. Due to the design of cecil's BaseAssemblyResolver, there is a small amount of duplication.
@marek-safar PTAL. |
return args.ToString (); | ||
} | ||
|
||
protected override string GenerateResponseFileCommands () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make the response file such that there is one argument/switch per line?
Basically add newlines at the end of each option?
This makes it easier to inspect the rsp files manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
Fixes #58. |
src/linker/Linker/Driver.cs
Outdated
@@ -240,6 +240,10 @@ public void Run (ILogger customLogger = null) | |||
disabled_optimizations.Add (opt); | |||
|
|||
continue; | |||
|
|||
case "--ref": | |||
context.Resolver.AddAssemblyPath (GetParam ()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need a different name for something what -d <path>
already covers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like --ref <path-to-file>
is necessary to prevent issues with-d <path-to-directory>
where different directories have dlls with the same name, but we only want to use one.
Specifically:
Our SDK passes the linker task a specific list of files as input. One of these is WindowsBase.dll
from a netcoreapp directory. Some other inputs (for WPF apps) live in a windowsdesktop directory, which also contains a WindowsBase.dll
(that we don't want to use). Before linking, the SDK already does conflict resolution to pick the right input. However, when the linker gets the directory of both inputs, it can resolve the wrong WindowsBase.dll
when it scans whole directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right so the intention is to --ref behave like
--reference /user/lib/file.dll
? It would be good if the names in the code actually reflected that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Which names? Maybe SearchAssemblyPaths -> SearchFilePaths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd start with the documentation, it should be
-reference <file> .....
similarly, with the resolver entry, replace AddAssemblyPath
with AddReferenceAssembly
or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
return base.Resolve (name, parameters); | ||
} | ||
|
||
private AssemblyDefinition SearchAssemblyPaths (AssemblyNameReference name, IEnumerable<string> assemblyPaths, ReaderParameters parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why base implementation cannot be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base classes currently only have logic to search in directories (BaseAssemblyResolver from cecil, or DirectoryAssemblyResolver from the linker). To move it to the base class, we would need to do one of:
- Modify cecil's resolution behavior. I've attempted this in the past, but Jb suggested that the linker should own any resolution logic specific to its purposes.
- Move it to DirectoryAssemblyResolver but not BaseAssemblyResolver, and support
--ref
only on illink, not on monolinker. - Keep it in the child class, extending both base classes.
I opted for the third approach, to keep the command-line consistent between illink/monolinker (I thought this option may be useful for monolinker as well, since it could in theory hit similar issues).
If you prefer, I can also make the change only for illink, and move the change to DirectoryAssemblyResolver only. This is actually what I did initially (see the first commit), before I realized that monolinker wouldn't support the change this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it the to ResolveFromReferences (AssemblyNameReference name, Collection<string> references, ReaderParameters parameters)
return base.Resolve (name, parameters); | ||
} | ||
|
||
private AssemblyDefinition SearchAssemblyPaths (AssemblyNameReference name, IEnumerable<string> assemblyPaths, ReaderParameters parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it the to ResolveFromReferences (AssemblyNameReference name, Collection<string> references, ReaderParameters parameters)
@@ -50,7 +50,7 @@ public virtual AssemblyDefinition Resolve (AssemblyNameReference name, ReaderPar | |||
if (name == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the ArgumentNullException check now redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're redundant in the sense that the base class performs the same check, but also necessary to prevent passing null to SearchAssemblyPaths (renamed to ResolveFromReferences).
You're right that we already deref name in Resolve, so maybe that's not strictly necessary, but I still feel that it's safer to keep both checks (moved to the beginning of Resolve).
--ref <path> -> --reference <file> SearchAssemblyPaths -> ResolveFromReferences AddAssemblyPath -> AddReferenceAssembly _assemblyPaths -> _references Also: - move parameter checking to beginning of Resolve () - remove unnecessary "System." - move computed string out of loop
/azp help |
Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run. Example: "run" or "run pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
* Resolve assemblies from specific paths This introduces a command-line option, "-reference <path>", which makes it possible to give the linker full assembly paths to resolve. LinkTask now uses "-reference" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves. When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands). * Move path resolution logic to child assembly resolver class This is necessary for the '--ref' argument to work with the mono build of the linker. Due to the design of cecil's BaseAssemblyResolver, there is a small amount of duplication. * Place response file arguments on separate lines * Move SearchAssemblyPaths call directly into Resolve * Update names for clarity and other PR feedback --ref <path> -> --reference <file> SearchAssemblyPaths -> ResolveFromReferences AddAssemblyPath -> AddReferenceAssembly _assemblyPaths -> _references Also: - move parameter checking to beginning of Resolve () - remove unnecessary "System." - move computed string out of loop * Update Driver.cs * Update Driver.cs * Update AssemblyResolver.cs * Update LinkTask.cs Commit migrated from dotnet/linker@923463b
This introduces a command-line option, "-reference ", which makes it possible to give the linker full assembly paths to resolve.
LinkTask now uses "-reference" instead of "-d", to prevent the directory resolution behavior from finding dependencies next to inputs. Now it takes an explicit list of inputs, which will be the only files it resolves.
When passing along full paths to each file, the command-line becomes substantially longer. To prevent hitting command length limits, arguments are now passed via a response file (using ToolTask's GenerateResponseFileCommands).
This fixes issues like that mentioned in https://github.com/dotnet/coreclr/issues/24397#issuecomment-490807112 when trying to link WPF apps, which have a
WindowsBase.dll
in the core framework directory and also in the WPF runtime pack: